Can we add APS metadata to result of the auction?

Summary

Replaces the legacy APS TAM/contextual path with a first-class OpenRTB provider, adds a sandboxed opaque-origin creative renderer, a Prebid-adapter rendering route, PBS coexistence safeguards, and an inventory-identity override. The security core is well-hardened and I verified rather than assumed it — allow-same-origin is never combined with allow-scripts (pinned by a regression test), the nonce/envelope contract matches byte-for-byte across Rust and JS, and the two CodeQL alerts were genuinely resolved. CI is green.

That said, I found 7 blocking defects — an auction-wide failure from one malformed bid, two ways APS demand can still reach Prebid Server, two blank-slot / consumed-impression bugs in the new rendering routes, a privacy leak in the identity override, and a silent config break on upgrade. Requesting changes.

A process note: the PR body's change table documents only the first commit. Two later commits ("Add APS inventory identity overrides", "Render APS bids from the trustedServer Prebid adapter") added whole subsystems it omits — worth refreshing before merge.

Most findings are filed as inline comments. The cross-cutting ones are below.

Blocking

🔧 wrench

• APS exclusion is case-sensitive and leaks via the head-insert path (prebid.rs:1427/1434, prebid.rs:909): Both PBS guards compare name == "aps" exactly, so "APS"/"Aps" in the free-form config.bidders slips through to PBS and is sold twice. Separately, head_inserts serializes config.bidders/client_side_bidders verbatim into window.__tsjs_prebid, so aps still ships to the browser — the safeguard is server-side only, and client_side_bidders is never filtered at all. The durable fix for this and the stored-request finding is to reject aps in [prebid].bidders/client_side_bidders at config-validation time.

Non-blocking

🤔 thinking

• Full client-controlled Referer path+query now enters the bidstream (formats.rs:93-116): site.page changed from a bare https://{domain} to the full same-origin Referer including query string, so publisher URLs carrying PII (?email=, session tokens, gclid) are forwarded to every SSP — up to 8 KiB of attacker-chosen data (the host check constrains only the authority). This mirrors client-side Prebid so it may be intentional, but for a privacy-preserving edge server it deserves an explicit decision: strip query/fragment by default, or gate behind config.
• adserver_mock mediation is last-write-wins and the new test enshrines it (adserver_mock.rs:107, :899): build_bid_index keys on (provider, slot, bidder); if APS ever stops reducing to one bid per slot, the mediator restores the losing candidate's renderer against the winning bid's price — wrong creative at wrong price, silently. bid_id now exists and would disambiguate; the mock only echoes crid. At least raise the collision log from debug! to warn!.
• providers.aps is now write-only and fails silently (creative_opportunities.rs:366,378): the field is a genuine deny_unknown_fields deser-compat shim (removing it hard-fails existing TOML), but to_ad_slot no longer reads it, so an operator who sets aps.slot_id expecting routing gets a no-op with zero diagnostics. Add a load-time warning.

♻️ refactor

• BidRenderer::aps() is an infallible accessor on a single-variant enum (types.rs:217-225, used at publisher.rs:1957): the moment a second variant lands the natural "fix" is a panic in the fallback arm. Prefer as_aps(&self) -> Option<&ApsRendererV1>. Note the generic bid.bid_id field this PR adds already carries the same value, so publisher.rs could derive hb_adid renderer-agnostically.

📝 note

• Undocumented /auction wire-contract change: for renderer bids, crid goes from always-present {bidder}-creative to bid.creative_id (absent when the bidder omits crid), id from {bidder}-{slot} to the upstream bid id, and adm is omitted entirely. tsjs absorbs all three, but POST /auction is a documented OpenRTB endpoint; any non-tsjs consumer reading those fields breaks. The CHANGELOG's Breaking section doesn't mention the response shape.
• Test quality across the new suites: the browser same-origin rejection test (aps-renderer.spec.ts:731-805) is effectively vacuous — a fixed waitForTimeout(100) with no positive control, driven through a hand-rolled harness instead of renderApsCreative, so it would pass if the guard broke. Several sandbox assertions (:464,:555) assert the harness's own input rather than product behavior. And there is no JS-side boundary coverage for the size caps (envelope 256 KiB / base64 349528, account-id 1024, creative-url 4096) nor a regression test for the two blocking rendering-route bugs. (I did confirm the same-origin guard at aps.rs:74 is live, not dead code, via a browser test — so that check is real; the test just doesn't exercise it.)

⛏ nitpick

• creative_id is the only renderer field without a length cap (aps.rs:682) — bounded by the 2 MiB body cap but inconsistent with account_id/creative_url.
• dropped_bid_count and drop_reasons don't reconcile (aps.rs:776-799) — price-losers increment the count with no reason entry; the test asserts dropped=1, drop_reasons={}.
• auction/README.md:120 ASCII box right border is off by one column.
• trusted-server.example.toml regressed a fictional aps.example.com/e/dtb/bid to a real vendor host; several docs/tests use real *.amazon-adsystem.com endpoints against the project's "example.com only" rule. These are functionally-required vendor endpoints (not customer data or credentials) and amazon-adsystem predates this PR, so noting rather than blocking.

CI Status

• fmt: PASS
• clippy (all six adapter targets): PASS
• rust tests (fastly/axum/cloudflare/spin + parity): PASS
• js tests (vitest): PASS
• browser integration + CodeQL: PASS

Summary

First-class APS OpenRTB integration replacing the legacy contextual/TAM path, with a sandboxed (opaque-origin, nonce-gated, CSP-enforced) renderer that deliberately withholds adm from the client outside that sandbox. The core security design (bid parsing never reads adm, envelope byte-bounds, iframe sandbox without allow-same-origin, identity-checked postMessage, server-side CSP header) is solid and well-tested, including a genuinely adversarial browser test suite (nonce replay, CSP-only isolation with the sandbox attribute omitted, live same-origin attack attempts). The main blocker is that this branch does not currently merge cleanly with main — see Blocking #1, which is also the root cause of the two failing CI checks.

Blocking

🔧 wrench

• PR does not merge cleanly with main: main's commit 8bb5450dc (#890) added a make_test_bid_with_creative() helper in publisher.rs that builds a Bid{} literal without this PR's new bid_id/creative_id/renderer fields. Rebasing/merging onto current main fails with error[E0063]: missing fields — this is exactly what's failing the cargo fmt and cargo test CI checks shown on this PR (GitHub builds the synthetic merge commit, not the raw branch tip). See inline comment on auction/types.rs.

❓ question

• Debug mode can leak adm to the browser (crates/trusted-server-core/src/integrations/aps.rs:628): undermines the PR's core claim that adm never reaches the client outside the sandbox. See inline comment.
• One bad bid can abort the entire /auction response (crates/trusted-server-core/src/auction/formats.rs:313): no multi-slot blast-radius test exists. See inline comment.

Non-blocking

♻️ refactor

• Bid (types.rs:253) has no #[derive(Default)] despite every field type supporting it — deriving it directly prevents recurrence of Blocking #1 the next time a field is added.
• aps.rs:1169 — no test exercises the fail-closed startup path (enabled+invalid config → register_providers() returning Err), only ApsConfig::validate() directly.
• gpt/index.ts:999 — renderingAdIds grows unbounded on the success path, inconsistent with sibling bounded caches added elsewhere in this PR.

🤔 thinking

• aps.rs:1047 — context-free parse_response silently discards a valid response if ever called without context; safe today only because all real call sites use parse_response_with_context.
• aps.rs:380 — device.language has no explicit byte cap, unlike other forwarded fields in this file.
• orchestrator.rs:28 — backend_to_provider's bare 4-tuple is duplicated across 3 sites; same fragility class as Blocking #1.
• formats.rs:302 — when both bid.creative and bid.renderer are Some, creative silently wins with no log/assert.
• prebid.rs:1427 — the "aps" bidder exclusion (preventing APS double-serving through Prebid Server) is case-sensitive; pre-existing pattern, worth eq_ignore_ascii_case given the stated goal.
• publisher.rs:2207 — full client query string is now forwarded into site.page for every provider; unlike aps.rs (8192-byte cap), prebid.rs's equivalent has no bound.
• render.ts:325 — rapid re-render of the same slot orphans the prior frame's message listener/timeout instead of routing through its own cleanup (self-heals within 10s).
• TESTING.md:49 — still describes APS as "mocked" / lists "Implement real APS" as a future step, contradicting this PR and the log excerpt this PR itself just updated a few lines above.
• docs/guide/auction-orchestration.md:623 — config reference table omits inventory_domain/inventory_page_origin, real validated fields documented elsewhere in this PR.

⛏ nitpick

• aps.rs:851 — a seatbid entry with no bid array is skipped without incrementing drop_reasons, making a no-bid response harder to diagnose.
• render.test.ts:216 — no test for "correct nonce, wrong event.source" rejection specifically (the guard is implemented correctly at render.ts:351, just not locked in by that exact case).
• render.ts:388 — the embedded renderer-document string hand-duplicates constants (including the 10000ms timeout) instead of interpolating them.

🌱 seedling

• Byte-limit constants (MAX_ACCOUNT_ID_BYTES, etc.) exist as 3 independently hardcoded copies across Rust, TS, and the embedded HTML string — worth a single source of truth eventually.
• creative_opportunities.rs:366 — legacy providers.aps.slot_id is now a silent no-op with no operator-facing warning if left configured.
• aps-renderer.spec.ts:169 — no browser-level test for cross-ad-unit capability theft (the messageSourceBelongsToAdUnit guard exists and works, just isn't e2e-tested against a mismatched ad unit).

📝 note

• auction/README.md:364 — this example is stale relative to the new renderer flow (pre-existing drift, not touched by this PR).

👍 praise

• aps.rs: parse_bid never reads adm; envelope is byte-bounded and fixture-verified byte-for-byte between Rust and TS; CSP header (APS_RENDERER_CSP) is correctly sent server-side with no allow-same-origin.
• orchestrator.rs: provider_request_context/effective_timeout threading fix, pinned by dispatched_collection_reuses_provider_launch_context.
• render.ts: validateApsRenderer's exact-key allowlist cross-checking the outer descriptor vs. the decoded OpenRTB payload; the event.source === iframe.contentWindow identity check (the right check for an opaque-origin sandbox).
• prebid.rs: the trustedServer bidder-expansion path correctly re-strips "aps" even when reintroduced via config.bidders, with a dedicated regression test.
• aps-renderer.spec.ts: nonce-replay, CSP-only isolation (with the sandbox attribute omitted), and same-origin attack tests are genuinely adversarial — real attempts against a live server, not tautological checks of config strings.

CI Status

• fmt: FAIL (caused by the merge-conflict issue above, not this branch's own code)
• rust tests: FAIL (same root cause)
• clippy (all adapters): PASS
• cross-adapter parity: PASS
• browser/integration tests: PASS
• vitest: PASS
• CodeQL: PASS

Summary

Re-reviewed at 10cd6d3ce. Every finding from the previous review is fixed — I verified all seven blocking items and the non-blocking ones in source, and several were implemented exactly as suggested. CI is green across all 19 checks.

Two new blocking issues, both introduced by work landed since the last review: the fix for the case-sensitive APS exclusion over-corrected into a config hard-fail that can take a service down on upgrade, and the new debug-metadata commit captures upstream response headers without the fail-closed filtering this codebase applies elsewhere. Both have small fixes.

Blocking

🔧 wrench

• The aps config rejection is an undocumented hard-fail that bricks every request (prebid.rs:346-368): config.validate() runs at settings.rs:257 before the is_enabled() check at line 265, and IntegrationRegistry::new (registry.rs:791) propagates the error with ?. An operator whose published config contains bidders = ["aps", "kargo"] — previously harmless, silently stripped — deploys this WASM and every request fails, including when the prebid integration is disabled entirely. The CHANGELOG Breaking entry says operators must "disable native APS demand" but never states the config will refuse to boot; docs/guide/configuration.md doesn't either. This PR's own tests construct ["kargo", "aps"], so it is a config the codebase expects to exist in the wild. The hard-fail is also redundant with the runtime filter added in the same commit (lines 1571/1578 already strip aps case-insensitively). Either downgrade to warn-and-strip, or keep the error and add a dedicated **Breaking** CHANGELOG line plus a docs migration note — and confirm ts config push rejects it before the runtime does.

Non-blocking

🤔 thinking

• Skipped winners are still counted as wins in telemetry (formats.rs:341,351): emit_auction_events_best_effort_lazy(... Completed { result }) runs at auction/endpoints.rs:320, before response conversion, and telemetry.rs:697-711 emits is_win = 1 per winning bid. A bid dropped by the new continue is reported as a win in analytics but never delivered, and endpoints.rs:331 still logs the pre-drop count. There's also no machine-readable drop counter — only a log::warn! — which is inconsistent with the drop_reasons metadata this same PR adds on the APS side. Consider counting skips into ResponseExt.orchestrator, or emitting the terminal event after conversion.
• An all-APS page now reports a Prebid provider error rather than a no-bid (prebid.rs:1598-1604): when every slot is APS-only the imp vector is empty and the existing guard returns Err(TrustedServerError::Prebid), surfacing as BidStatus::Error in provider_details. That's a normal configuration, not a failure, and it will pollute provider error rates and alerting. Suggest an explicit no-bid response for "all imps intentionally dropped", reserving Err for "no valid banner formats".
• Debug body capture inherits the 2 MiB upstream cap (aps.rs:945-956, 970-978): captured bytes pass through String::from_utf8_lossy (3 bytes per invalid byte) and then JSON escaping, so an adversarial 2 MiB upstream body can inflate the /auction response to several MB, buffered in WASM heap. Per-auction rather than per-bid, so it doesn't accumulate, but a dedicated smaller preview cap mirroring MAX_BID_CREATIVE_DUMP_BYTES would be safer.
• The Referer privacy fix is only half-applied: /auction sanitizes, but the SSR path builds publisher.page_url at publisher.rs:2018 as scheme://host{request_path_and_query} — full query string and edge host, unfiltered — and passes it onward. Worth routing both producers through one sanitizer. Related: HeaderValue::from_str failing on a non-ASCII URL silently drops Referer with no log.
• Debug requestbody is reconstructed, not captured (aps.rs:596-605): debug_request() re-runs build_openrtb_request at parse time and presents the result as what was sent. That holds only while the function stays pure. If it ever gains a uuid, timestamp, or any mutation between build and send, debug will silently report a payload APS never received and an operator will chase a fabricated request. Worth a test asserting the debug body byte-matches the captured outbound body, plus a doc comment recording the invariant.
• One browser assertion passes vacuously (aps-renderer.spec.ts:364-366): expect(result.foreignUniversalCreativeResponse).not.toEqual(expect.objectContaining({ apsRenderer })) succeeds when the promise times out to undefined, and would also succeed if the bridge served a slightly different renderer. The intended contract is "a foreign frame gets nothing" — assert toBeUndefined().

♻️ refactor

• validateApsRenderer now runs 3–4× per APS bid on the critical path: admission (prebid/index.ts:213), registration (render.ts:210), and the GPT bridge (gpt/index.ts:998). Each pass allocates a TextEncoder, base64-decodes up to 256 KB, and JSON.parses it. The admission call is new. Consider caching the validated descriptor on first success.
• Four-element anonymous tuple in backend_to_provider (orchestrator.rs:28, 460, 877-880): (String, Instant, Arc<dyn AuctionProvider>, u32) destructured as (provider_name, start_time, _, _) at five call sites, with an un-self-describing u32. A named struct would match the convention the project applies to argument lists.

📝 note

• Two trade-offs here came from my own suggestions last round — flagging so they get an explicit decision rather than sliding in as side effects. (1) Suffix host matching (formats.rs:110-119) is implemented correctly — the prefix.ends_with('.') check properly rejects evilpublisher.example — but it does mean any subdomain can now determine the site.page every SSP sees; on a deployment with user-content subdomains that is worth a conscious accept. (2) Unconditional set_query(None) is good for privacy but collapses every page of a query-driven site (/index.php?id=123) to a single URL, degrading contextual targeting and per-page reporting. Consider a key allowlist or a config switch, and document the trade-off either way.
• debug flag discoverability (aps.rs:132-136): the one-line doc comment carries no risk warning, whereas the analogous flags in settings.rs:1897-1923 each have multi-line "never enable in production" rustdoc. An operator auditing the [debug] section also won't find this switch, since it lives under [integrations.aps]. Worth mirroring the stronger wording.

⛏ nitpick

• attach_debug_metadata is skipped only on the missing_request_context branch (aps.rs:996-1002), contradicting the docs' claim that the exchange is emitted for success, 204, malformed and non-success statuses. Unreachable today, so latent rather than live.
• The debug requestheaders map is a hardcoded content-type: application/json (aps.rs:604-608) rather than the headers actually sent at line 1042. They agree today and will drift; it also slightly misrepresents the PBS httpcalls shape it mirrors.
• On failed renderer registration the descriptor is now retained on the bid (prebid/index.ts:546-552). I checked reachability and it is effectively defensive-only — admission already validated the descriptor, adId is Prebid-generated, and validPrebidIdentity accepts GAM-style codes — so this is hygiene, not a live bug.
• publisher.rs:2139 still uses expect("should serialize typed renderer") while formats.rs:331-342 was hardened to handle the identical serialization failing; worth resolving the asymmetry in one direction.

CI Status

• fmt: PASS
• clippy (all six adapter targets): PASS
• rust tests (fastly/axum/cloudflare/spin + parity): PASS
• js tests (vitest): PASS
• browser integration + CodeQL: PASS